feat: ABAC Rooms attributes tab #37408
Conversation
|
Looks like this PR is not ready to merge, because of the following issues:
Please fix the issues and try again If you have any trouble, please check the PR guidelines |
|
|
Warning Rate limit exceeded@MartinSchoeler has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 13 minutes and 36 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (2)
WalkthroughAdds ABAC room-attributes admin: query keys, list/search/pagination UI, create/edit contextual bars, kebab menu with edit/delete flows (including in-use check and confirmations), availability hook, i18n keys, schema typing tweak, sidebar permission fix, and related tests. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Page as AdminABACPage
participant List as AdminABACRoomAttributes
participant Context as RoomAttributesContextualBar
participant API
participant Cache as QueryCache
User->>Page: open admin ABAC (tab=room-attributes)
Page->>List: render list view
List->>API: GET /v1/abac/attributes?search/offset/count
API-->>List: attributes + total
User->>List: Click "New" or "Edit"
List->>Page: set route context (new / edit + id)
Page->>Context: open contextual bar
alt edit
Context->>API: GET /v1/abac/attributes/:id
API-->>Context: attribute
end
User->>Context: submit form
Context->>API: POST /v1/abac/attributes or PUT /v1/abac/attributes/:id
API-->>Context: success
Context->>Cache: invalidate ABACQueryKeys.roomAttributes.all()
Context->>User: show success toast, close contextual bar
Page->>List: refreshed data
sequenceDiagram
participant User
participant Menu as AdminABACRoomAttributeMenu
participant Hook as useRoomAttributeItems
participant API
participant Modal
participant Toast
participant Cache as QueryCache
User->>Menu: open menu, choose "Edit"
Menu->>Hook: navigate to edit context (route with id)
User->>Menu: choose "Delete"
Hook->>API: GET /v1/abac/attributes/:id/in-use
API-->>Hook: in-use? true/false
alt in-use
Hook->>Modal: show warning (cannot delete)
else not in-use
Hook->>Modal: show confirmation
User->>Modal: confirm
Hook->>API: DELETE /v1/abac/attributes/:id
API-->>Hook: success
Hook->>Toast: show deleted message
Hook->>Cache: invalidate ABACQueryKeys.roomAttributes.all()
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (2 inconclusive)
✅ Passed checks (3 passed)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
MartinSchoeler
force-pushed
the
geat/ABAC-admin-room-att
branch
from
4a62d59 to
2edb0d6
Compare
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## feat/abac #37408 +/- ##
=============================================
- Coverage 54.27% 54.26% -0.02%
=============================================
Files 2658 2658
Lines 49964 49968 +4
Branches 11126 11126
=============================================
- Hits 27120 27116 -4
- Misses 20710 20717 +7
- Partials 2134 2135 +1
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
MartinSchoeler
force-pushed
the
geat/ABAC-admin-room-att
branch
2 times, most recently
from
46600a8 to
8aa164f
Compare
MartinSchoeler
force-pushed
the
geat/ABAC-admin-room-att
branch
from
8aa164f to
273d408
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/meteor/client/views/admin/ABAC/AdminABACPage.tsx (1)
30-74: Closing the contextual bar leaves an empty drawer
Settingcontext/idto empty strings keeps the contextual bar mounted (thecontext !== undefinedcheck still passes) and the close handler then short-circuits on the falsy value. After any close, you get a blank contextual bar you can’t dismiss. Please strip those params instead of blanking them so the route truly resets.const handleCloseContextualbar = useEffectEvent((): void => { - if (!context) { + if (context === undefined) { return; } - router.navigate( - { - name: 'admin-ABAC', - params: { ...router.getRouteParameters(), context: '', id: '' }, - }, - { replace: true }, - ); + const { context: _context, id: _id, ...params } = router.getRouteParameters(); + + router.navigate( + { + name: 'admin-ABAC', + params, + }, + { replace: true }, + ); });
🧹 Nitpick comments (1)
apps/meteor/client/views/admin/ABAC/RoomAttributesContextualBar.tsx (1)
13-21: Remove unusedattributeIdprop.The
attributeIdprop is defined in the interface but never used in the component. Instead, the component readsattributeIdfrom the route parameter on line 47. Either use the prop or remove it from the interface.Apply this diff to remove the unused prop:
type RoomAttributesContextualBarProps = { - attributeId?: string; attributeData?: { key: string; values: string[]; usage: Record<string, boolean>; }; onClose: () => void; };
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Jira integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
⛔ Files ignored due to path filters (1)
apps/meteor/client/views/admin/ABAC/__snapshots__/AdminABACRoomAttributesForm.spec.tsx.snapis excluded by!**/*.snap
📒 Files selected for processing (15)
apps/meteor/client/lib/queryKeys.ts(1 hunks)apps/meteor/client/views/admin/ABAC/AdminABACPage.tsx(3 hunks)apps/meteor/client/views/admin/ABAC/AdminABACRoomAttributeMenu.tsx(1 hunks)apps/meteor/client/views/admin/ABAC/AdminABACRoomAttributes.tsx(1 hunks)apps/meteor/client/views/admin/ABAC/AdminABACRoomAttributesForm.spec.tsx(4 hunks)apps/meteor/client/views/admin/ABAC/AdminABACRoomAttributesForm.tsx(2 hunks)apps/meteor/client/views/admin/ABAC/AdminABACTabs.tsx(1 hunks)apps/meteor/client/views/admin/ABAC/RoomAttributesContextualBar.tsx(1 hunks)apps/meteor/client/views/admin/ABAC/RoomAttributesContextualBarWithData.tsx(1 hunks)apps/meteor/client/views/admin/ABAC/hooks/useIsABACAvailable.tsx(1 hunks)apps/meteor/client/views/admin/ABAC/useRoomAttributeOptions.spec.tsx(1 hunks)apps/meteor/client/views/admin/ABAC/useRoomAttributeOptions.tsx(1 hunks)apps/meteor/client/views/admin/sidebarItems.ts(1 hunks)apps/meteor/ee/server/api/abac/schemas.ts(1 hunks)packages/i18n/src/locales/en.i18n.json(1 hunks)
🧰 Additional context used
🧠 Learnings (19)
📓 Common learnings
Learnt from: KevLehman
Repo: RocketChat/Rocket.Chat PR: 37303
File: apps/meteor/tests/end-to-end/api/abac.ts:1125-1137
Timestamp: 2025-10-27T14:38:46.994Z
Learning: In Rocket.Chat ABAC feature, when ABAC is disabled globally (ABAC_Enabled setting is false), room-level ABAC attributes are not evaluated when changing room types. This means converting a private room to public will succeed even if the room has ABAC attributes, as long as the global ABAC setting is disabled.
Learnt from: KevLehman
Repo: RocketChat/Rocket.Chat PR: 37299
File: apps/meteor/ee/server/lib/ldap/Manager.ts:438-454
Timestamp: 2025-10-24T17:32:05.348Z
Learning: In Rocket.Chat, ABAC attributes can only be set on private rooms and teams (type 'p'), not on public rooms (type 'c'). Therefore, when checking for ABAC-protected rooms/teams during LDAP sync or similar operations, it's sufficient to query only private rooms using methods like `findPrivateRoomsByIdsWithAbacAttributes`.
📚 Learning: 2025-11-07T14:50:33.544Z
Learnt from: KevLehman
Repo: RocketChat/Rocket.Chat PR: 37423
File: packages/i18n/src/locales/en.i18n.json:18-18
Timestamp: 2025-11-07T14:50:33.544Z
Learning: Rocket.Chat settings: in apps/meteor/ee/server/settings/abac.ts, the Abac_Cache_Decision_Time_Seconds setting uses invalidValue: 0 as the fallback when ABAC is unlicensed. With a valid license, admins can still set the value to 0 to intentionally disable the ABAC decision cache.
Applied to files:
apps/meteor/ee/server/api/abac/schemas.tsapps/meteor/client/views/admin/ABAC/hooks/useIsABACAvailable.tsxapps/meteor/client/views/admin/sidebarItems.ts
📚 Learning: 2025-10-30T19:30:46.541Z
Learnt from: MartinSchoeler
Repo: RocketChat/Rocket.Chat PR: 37244
File: apps/meteor/client/views/admin/ABAC/AdminABACRoomAttributesForm.spec.tsx:125-146
Timestamp: 2025-10-30T19:30:46.541Z
Learning: In the AdminABACRoomAttributesForm component (apps/meteor/client/views/admin/ABAC/AdminABACRoomAttributesForm.tsx), the first attribute value field is mandatory and does not have a Remove button. Only additional values beyond the first have Remove buttons. This means trashButtons[0] corresponds to the second value's Remove button, not the first value's.
Applied to files:
apps/meteor/client/views/admin/ABAC/RoomAttributesContextualBar.tsxapps/meteor/client/views/admin/ABAC/useRoomAttributeOptions.tsxapps/meteor/client/views/admin/ABAC/AdminABACRoomAttributesForm.tsxapps/meteor/client/views/admin/ABAC/RoomAttributesContextualBarWithData.tsxapps/meteor/client/views/admin/ABAC/AdminABACRoomAttributes.tsxapps/meteor/client/views/admin/ABAC/AdminABACRoomAttributesForm.spec.tsxapps/meteor/client/views/admin/ABAC/AdminABACPage.tsxapps/meteor/client/views/admin/ABAC/AdminABACTabs.tsxapps/meteor/client/views/admin/ABAC/AdminABACRoomAttributeMenu.tsx
📚 Learning: 2025-10-27T14:38:46.994Z
Learnt from: KevLehman
Repo: RocketChat/Rocket.Chat PR: 37303
File: apps/meteor/tests/end-to-end/api/abac.ts:1125-1137
Timestamp: 2025-10-27T14:38:46.994Z
Learning: In Rocket.Chat ABAC feature, when ABAC is disabled globally (ABAC_Enabled setting is false), room-level ABAC attributes are not evaluated when changing room types. This means converting a private room to public will succeed even if the room has ABAC attributes, as long as the global ABAC setting is disabled.
Applied to files:
apps/meteor/client/views/admin/ABAC/RoomAttributesContextualBar.tsxapps/meteor/client/lib/queryKeys.tsapps/meteor/client/views/admin/ABAC/hooks/useIsABACAvailable.tsxapps/meteor/client/views/admin/ABAC/AdminABACRoomAttributes.tsxpackages/i18n/src/locales/en.i18n.jsonapps/meteor/client/views/admin/ABAC/AdminABACPage.tsxapps/meteor/client/views/admin/ABAC/AdminABACTabs.tsxapps/meteor/client/views/admin/sidebarItems.ts
📚 Learning: 2025-10-24T17:32:05.348Z
Learnt from: KevLehman
Repo: RocketChat/Rocket.Chat PR: 37299
File: apps/meteor/ee/server/lib/ldap/Manager.ts:438-454
Timestamp: 2025-10-24T17:32:05.348Z
Learning: In Rocket.Chat, ABAC attributes can only be set on private rooms and teams (type 'p'), not on public rooms (type 'c'). Therefore, when checking for ABAC-protected rooms/teams during LDAP sync or similar operations, it's sufficient to query only private rooms using methods like `findPrivateRoomsByIdsWithAbacAttributes`.
Applied to files:
apps/meteor/client/lib/queryKeys.tsapps/meteor/client/views/admin/ABAC/AdminABACPage.tsx
📚 Learning: 2025-09-16T22:08:51.490Z
Learnt from: CR
Repo: RocketChat/Rocket.Chat PR: 0
File: .cursor/rules/playwright.mdc:0-0
Timestamp: 2025-09-16T22:08:51.490Z
Learning: Applies to apps/meteor/tests/e2e/**/*.spec.ts : Use descriptive test names that clearly communicate expected behavior
Applied to files:
apps/meteor/client/views/admin/ABAC/useRoomAttributeOptions.spec.tsxapps/meteor/client/views/admin/ABAC/AdminABACRoomAttributesForm.spec.tsx
📚 Learning: 2025-09-16T22:08:51.490Z
Learnt from: CR
Repo: RocketChat/Rocket.Chat PR: 0
File: .cursor/rules/playwright.mdc:0-0
Timestamp: 2025-09-16T22:08:51.490Z
Learning: Applies to apps/meteor/tests/e2e/**/*.spec.ts : Maintain test isolation between test cases
Applied to files:
apps/meteor/client/views/admin/ABAC/useRoomAttributeOptions.spec.tsxapps/meteor/client/views/admin/ABAC/AdminABACRoomAttributesForm.spec.tsx
📚 Learning: 2025-09-16T22:08:51.490Z
Learnt from: CR
Repo: RocketChat/Rocket.Chat PR: 0
File: .cursor/rules/playwright.mdc:0-0
Timestamp: 2025-09-16T22:08:51.490Z
Learning: Applies to apps/meteor/tests/e2e/**/*.spec.ts : Utilize Playwright fixtures (test, page, expect) consistently
Applied to files:
apps/meteor/client/views/admin/ABAC/useRoomAttributeOptions.spec.tsx
📚 Learning: 2025-09-16T22:08:51.490Z
Learnt from: CR
Repo: RocketChat/Rocket.Chat PR: 0
File: .cursor/rules/playwright.mdc:0-0
Timestamp: 2025-09-16T22:08:51.490Z
Learning: Applies to apps/meteor/tests/e2e/**/*.spec.ts : Use expect matchers (toEqual, toContain, toBeTruthy, toHaveLength, etc.) instead of assert statements
Applied to files:
apps/meteor/client/views/admin/ABAC/useRoomAttributeOptions.spec.tsxapps/meteor/client/views/admin/ABAC/AdminABACRoomAttributesForm.spec.tsx
📚 Learning: 2025-09-16T22:08:51.490Z
Learnt from: CR
Repo: RocketChat/Rocket.Chat PR: 0
File: .cursor/rules/playwright.mdc:0-0
Timestamp: 2025-09-16T22:08:51.490Z
Learning: Applies to apps/meteor/tests/e2e/**/*.spec.ts : Group related tests in the same file
Applied to files:
apps/meteor/client/views/admin/ABAC/useRoomAttributeOptions.spec.tsxapps/meteor/client/views/admin/ABAC/AdminABACRoomAttributesForm.spec.tsx
📚 Learning: 2025-09-16T22:08:51.490Z
Learnt from: CR
Repo: RocketChat/Rocket.Chat PR: 0
File: .cursor/rules/playwright.mdc:0-0
Timestamp: 2025-09-16T22:08:51.490Z
Learning: Applies to apps/meteor/tests/e2e/**/*.spec.ts : Ensure tests run reliably in parallel without shared state conflicts
Applied to files:
apps/meteor/client/views/admin/ABAC/useRoomAttributeOptions.spec.tsxapps/meteor/client/views/admin/ABAC/AdminABACRoomAttributesForm.spec.tsx
📚 Learning: 2025-09-16T22:08:51.490Z
Learnt from: CR
Repo: RocketChat/Rocket.Chat PR: 0
File: .cursor/rules/playwright.mdc:0-0
Timestamp: 2025-09-16T22:08:51.490Z
Learning: Applies to apps/meteor/tests/e2e/**/*.{ts,tsx,js,jsx} : Write concise, technical TypeScript/JavaScript with accurate typing
Applied to files:
apps/meteor/client/views/admin/ABAC/useRoomAttributeOptions.spec.tsx
📚 Learning: 2025-09-16T22:08:51.490Z
Learnt from: CR
Repo: RocketChat/Rocket.Chat PR: 0
File: .cursor/rules/playwright.mdc:0-0
Timestamp: 2025-09-16T22:08:51.490Z
Learning: Applies to apps/meteor/tests/e2e/**/*.spec.ts : Prefer web-first assertions (e.g., toBeVisible, toHaveText)
Applied to files:
apps/meteor/client/views/admin/ABAC/useRoomAttributeOptions.spec.tsxapps/meteor/client/views/admin/ABAC/AdminABACRoomAttributesForm.spec.tsx
📚 Learning: 2025-09-16T22:08:51.490Z
Learnt from: CR
Repo: RocketChat/Rocket.Chat PR: 0
File: .cursor/rules/playwright.mdc:0-0
Timestamp: 2025-09-16T22:08:51.490Z
Learning: Applies to apps/meteor/tests/e2e/**/*.spec.ts : All Playwright test files must be located under apps/meteor/tests/e2e/ and use the .spec.ts extension (e.g., login.spec.ts)
Applied to files:
apps/meteor/client/views/admin/ABAC/useRoomAttributeOptions.spec.tsxapps/meteor/client/views/admin/ABAC/AdminABACRoomAttributesForm.spec.tsx
📚 Learning: 2025-09-16T22:08:51.490Z
Learnt from: CR
Repo: RocketChat/Rocket.Chat PR: 0
File: .cursor/rules/playwright.mdc:0-0
Timestamp: 2025-09-16T22:08:51.490Z
Learning: Applies to apps/meteor/tests/e2e/**/*.{ts,tsx,js,jsx} : Follow DRY by extracting reusable logic into helper functions or page objects
Applied to files:
apps/meteor/client/views/admin/ABAC/useRoomAttributeOptions.spec.tsx
📚 Learning: 2025-09-16T22:08:51.490Z
Learnt from: CR
Repo: RocketChat/Rocket.Chat PR: 0
File: .cursor/rules/playwright.mdc:0-0
Timestamp: 2025-09-16T22:08:51.490Z
Learning: Applies to apps/meteor/tests/e2e/**/*.{ts,tsx} : Avoid using page.locator(); prefer semantic locators like page.getByRole, page.getByLabel, page.getByText, and page.getByTitle
Applied to files:
apps/meteor/client/views/admin/ABAC/AdminABACRoomAttributesForm.spec.tsx
📚 Learning: 2025-09-16T22:08:51.490Z
Learnt from: CR
Repo: RocketChat/Rocket.Chat PR: 0
File: .cursor/rules/playwright.mdc:0-0
Timestamp: 2025-09-16T22:08:51.490Z
Learning: Applies to apps/meteor/tests/e2e/**/*.spec.ts : Ensure a clean state for each test execution
Applied to files:
apps/meteor/client/views/admin/ABAC/AdminABACRoomAttributesForm.spec.tsx
📚 Learning: 2025-09-16T22:08:51.490Z
Learnt from: CR
Repo: RocketChat/Rocket.Chat PR: 0
File: .cursor/rules/playwright.mdc:0-0
Timestamp: 2025-09-16T22:08:51.490Z
Learning: Applies to apps/meteor/tests/e2e/**/*.{ts,tsx} : Follow the Page Object Model pattern consistently
Applied to files:
apps/meteor/client/views/admin/ABAC/AdminABACRoomAttributesForm.spec.tsx
📚 Learning: 2025-09-23T19:22:59.217Z
Learnt from: dougfabris
Repo: RocketChat/Rocket.Chat PR: 36987
File: apps/meteor/tests/e2e/page-objects/fragments/room-toolbar.ts:10-20
Timestamp: 2025-09-23T19:22:59.217Z
Learning: In Playwright e2e tests, prefer stable selectors like data-qa-id attributes over localized text in getByRole() or getByText() calls to prevent test failures when UI language changes. Test translations separately by validating actual text content after ensuring UI interactions work with stable selectors.
Applied to files:
apps/meteor/client/views/admin/ABAC/AdminABACRoomAttributesForm.spec.tsx
🧬 Code graph analysis (8)
apps/meteor/client/views/admin/ABAC/RoomAttributesContextualBar.tsx (4)
packages/ui-contexts/src/index.ts (2)
useRouteParameter(62-62)useToastMessageDispatch(75-75)apps/meteor/client/views/admin/ABAC/AdminABACRoomAttributesForm.tsx (1)
AdminABACRoomAttributesFormFormData(19-23)apps/meteor/client/lib/queryKeys.ts (1)
ABACQueryKeys(121-128)apps/meteor/client/components/Contextualbar/index.ts (3)
ContextualbarHeader(23-23)ContextualbarTitle(33-33)ContextualbarClose(28-28)
apps/meteor/client/views/admin/ABAC/useRoomAttributeOptions.tsx (1)
apps/meteor/client/lib/queryKeys.ts (1)
ABACQueryKeys(121-128)
apps/meteor/client/views/admin/ABAC/useRoomAttributeOptions.spec.tsx (2)
packages/mock-providers/src/index.ts (1)
mockAppRoot(3-3)apps/meteor/tests/mocks/data.ts (1)
createFakeLicenseInfo(213-261)
apps/meteor/client/views/admin/ABAC/AdminABACRoomAttributesForm.tsx (2)
packages/livechat/src/components/Button/index.tsx (1)
Button(34-99)packages/livechat/src/components/ButtonGroup/index.tsx (1)
ButtonGroup(8-14)
apps/meteor/client/views/admin/ABAC/RoomAttributesContextualBarWithData.tsx (2)
apps/meteor/client/lib/queryKeys.ts (1)
ABACQueryKeys(121-128)apps/meteor/client/components/Contextualbar/index.ts (1)
ContextualbarSkeletonBody(38-38)
apps/meteor/client/views/admin/ABAC/hooks/useIsABACAvailable.tsx (1)
packages/ui-contexts/src/index.ts (1)
useSetting(69-69)
apps/meteor/client/views/admin/ABAC/AdminABACRoomAttributes.tsx (2)
apps/meteor/client/components/GenericTable/hooks/usePagination.ts (1)
usePagination(11-40)apps/meteor/client/lib/queryKeys.ts (1)
ABACQueryKeys(121-128)
apps/meteor/client/views/admin/ABAC/AdminABACPage.tsx (3)
packages/ui-contexts/src/index.ts (2)
useRouter(60-60)useRouteParameter(62-62)apps/meteor/client/router/page.ts (1)
Page(92-241)apps/meteor/client/components/Contextualbar/index.ts (1)
ContextualbarDialog(22-22)
🔇 Additional comments (16)
apps/meteor/ee/server/api/abac/schemas.ts (1)
67-69: Type correction correctly aligns TypeScript types with AJV schema behavior.The change makes
keyandvaluesoptional in the TypeScript generic type, matching the underlying AJV schema which does not include them in arequiredarray. Properties keyword does not require the presence of any properties, you need to use required keyword, confirming that the AJV schema permits these fields to be optional at runtime. The TypeScript type now accurately reflects the actual validation behavior.apps/meteor/client/views/admin/ABAC/RoomAttributesContextualBar.tsx (1)
85-100: LGTM! Clean component structure.The JSX structure is well-organized with proper localization and separation of concerns. The dynamic title and description based on the edit/create mode is a good UX pattern.
apps/meteor/client/views/admin/ABAC/AdminABACRoomAttributesForm.tsx (5)
19-29: Good type safety improvement.Changing the
onSaveparameter type fromunknowntoAdminABACRoomAttributesFormFormDataprovides better type safety and makes the component's contract clearer.
75-87: Consider if dual error display is necessary.The name field displays errors in two places:
- Line 81:
error={errors.name?.message}on the TextInput component- Line 86:
<FieldError>{errors.name?.message || ''}</FieldError>below the inputThis might result in the error message appearing twice. Verify if this is the intended UX pattern in your design system, or if one of these should be removed.
92-114: LGTM! Correct field array logic.The remove button logic correctly ensures at least one value field remains:
- Locked attributes: first field is locked and cannot be removed (line 100)
- New attribute values: first field cannot be removed unless there are locked attributes (line 110)
This aligns with the requirement that attributes must have at least one value. Based on learnings.
116-127: LGTM! Comprehensive validation for adding values.The Add Value button's disabled logic properly prevents adding new fields when:
- Existing validation errors are present
- Any current field is empty
- The 10-value limit is reached (counting both locked and new values)
130-138: Good use of form attribute for external submission.Using
form={formId}on the submit button is a clean pattern that allows the button to be placed outside the form element while still triggering submission. The disabled logic properly prevents submission when there are validation errors.apps/meteor/client/views/admin/ABAC/useRoomAttributeOptions.tsx (3)
12-34: LGTM! Clean hook setup with proper callback stability.The hook is well-structured with proper use of
useEffectEventfor stable callbacks andreplace: truefor navigation, which is appropriate for contextual bar routing scenarios.
36-48: LGTM! Proper mutation with cache invalidation.The delete mutation correctly:
- Displays success/error toasts
- Invalidates the room attributes cache to refresh the list
- Closes the modal after completion
91-100: LGTM! Well-structured menu items with proper accessibility.The menu items array is well-structured with:
- Proper typing with
as constfor icon values- Accessibility via color coding (danger for delete)
- ABAC availability check for edit action
- Clear action callbacks
apps/meteor/client/views/admin/ABAC/AdminABACRoomAttributes.tsx (3)
56-70: LGTM! Clean search and action bar.The top action bar is well-structured with:
- Search input with visual indicator
- New attribute button properly gated by ABAC availability
- Good spacing and layout
71-106: LGTM! Proper table rendering with loading states.The table rendering correctly:
- Handles loading states and empty data
- Displays attribute values as comma-separated list
- Integrates pagination with proper count fallback
- Includes per-row action menu
42-49: No issues found - the query structure is intentional.The query object constructs an OR search across both
keyandvaluesfields using MongoDB's$oroperator. When both parameters are set todebouncedText, it correctly performs a regex search that matches attributes where either the key or any value matches the search text. This is reasonable behavior for a general search interface and works as designed.apps/meteor/client/views/admin/ABAC/useRoomAttributeOptions.spec.tsx (3)
1-61: LGTM! Comprehensive test setup with proper isolation.The test setup is well-structured with:
- Proper module mocking for UI contexts
- Reusable base app root configuration
- Mock cleanup in
beforeEachfor test isolation- Complete translations and endpoint mocks
62-140: LGTM! Thorough testing of edit functionality.The edit action tests comprehensively cover:
- Menu item structure validation
- ABAC availability gating (both enabled and disabled)
- Navigation with correct parameters
- Proper disabled state when ABAC is unavailable
142-261: LGTM! Comprehensive delete flow testing.The delete action tests thoroughly cover:
- Warning modal when attribute is in use
- Delete confirmation modal when attribute is not in use
- Actual endpoint invocation on confirmation
- Success toast with correct message
The
confirmHandlerpattern (lines 191-197, 227-233) is a clean approach to testing modal confirmation flows.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Jira integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (3)
apps/meteor/ee/server/api/abac/index.ts(3 hunks)apps/meteor/tests/end-to-end/api/abac.ts(1 hunks)packages/i18n/src/locales/en.i18n.json(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/i18n/src/locales/en.i18n.json
🧰 Additional context used
🧠 Learnings (10)
📓 Common learnings
Learnt from: KevLehman
Repo: RocketChat/Rocket.Chat PR: 37303
File: apps/meteor/tests/end-to-end/api/abac.ts:1125-1137
Timestamp: 2025-10-27T14:38:46.994Z
Learning: In Rocket.Chat ABAC feature, when ABAC is disabled globally (ABAC_Enabled setting is false), room-level ABAC attributes are not evaluated when changing room types. This means converting a private room to public will succeed even if the room has ABAC attributes, as long as the global ABAC setting is disabled.
Learnt from: KevLehman
Repo: RocketChat/Rocket.Chat PR: 37299
File: apps/meteor/ee/server/lib/ldap/Manager.ts:438-454
Timestamp: 2025-10-24T17:32:05.348Z
Learning: In Rocket.Chat, ABAC attributes can only be set on private rooms and teams (type 'p'), not on public rooms (type 'c'). Therefore, when checking for ABAC-protected rooms/teams during LDAP sync or similar operations, it's sufficient to query only private rooms using methods like `findPrivateRoomsByIdsWithAbacAttributes`.
📚 Learning: 2025-10-27T14:38:46.994Z
Learnt from: KevLehman
Repo: RocketChat/Rocket.Chat PR: 37303
File: apps/meteor/tests/end-to-end/api/abac.ts:1125-1137
Timestamp: 2025-10-27T14:38:46.994Z
Learning: In Rocket.Chat ABAC feature, when ABAC is disabled globally (ABAC_Enabled setting is false), room-level ABAC attributes are not evaluated when changing room types. This means converting a private room to public will succeed even if the room has ABAC attributes, as long as the global ABAC setting is disabled.
Applied to files:
apps/meteor/tests/end-to-end/api/abac.tsapps/meteor/ee/server/api/abac/index.ts
📚 Learning: 2025-09-16T22:08:51.490Z
Learnt from: CR
Repo: RocketChat/Rocket.Chat PR: 0
File: .cursor/rules/playwright.mdc:0-0
Timestamp: 2025-09-16T22:08:51.490Z
Learning: Applies to apps/meteor/tests/e2e/**/*.spec.ts : Ensure tests run reliably in parallel without shared state conflicts
Applied to files:
apps/meteor/tests/end-to-end/api/abac.ts
📚 Learning: 2025-09-16T22:08:51.490Z
Learnt from: CR
Repo: RocketChat/Rocket.Chat PR: 0
File: .cursor/rules/playwright.mdc:0-0
Timestamp: 2025-09-16T22:08:51.490Z
Learning: Applies to apps/meteor/tests/e2e/**/*.spec.ts : Use descriptive test names that clearly communicate expected behavior
Applied to files:
apps/meteor/tests/end-to-end/api/abac.ts
📚 Learning: 2025-09-16T22:08:51.490Z
Learnt from: CR
Repo: RocketChat/Rocket.Chat PR: 0
File: .cursor/rules/playwright.mdc:0-0
Timestamp: 2025-09-16T22:08:51.490Z
Learning: Applies to apps/meteor/tests/e2e/**/*.spec.ts : Maintain test isolation between test cases
Applied to files:
apps/meteor/tests/end-to-end/api/abac.ts
📚 Learning: 2025-09-16T22:08:51.490Z
Learnt from: CR
Repo: RocketChat/Rocket.Chat PR: 0
File: .cursor/rules/playwright.mdc:0-0
Timestamp: 2025-09-16T22:08:51.490Z
Learning: Applies to apps/meteor/tests/e2e/**/*.spec.ts : Group related tests in the same file
Applied to files:
apps/meteor/tests/end-to-end/api/abac.ts
📚 Learning: 2025-09-16T22:08:51.490Z
Learnt from: CR
Repo: RocketChat/Rocket.Chat PR: 0
File: .cursor/rules/playwright.mdc:0-0
Timestamp: 2025-09-16T22:08:51.490Z
Learning: Applies to apps/meteor/tests/e2e/**/*.{ts,tsx,js,jsx} : Write concise, technical TypeScript/JavaScript with accurate typing
Applied to files:
apps/meteor/tests/end-to-end/api/abac.ts
📚 Learning: 2025-09-16T22:08:51.490Z
Learnt from: CR
Repo: RocketChat/Rocket.Chat PR: 0
File: .cursor/rules/playwright.mdc:0-0
Timestamp: 2025-09-16T22:08:51.490Z
Learning: Applies to apps/meteor/tests/e2e/**/*.{ts,tsx,js,jsx} : Avoid code comments in the implementation
Applied to files:
apps/meteor/tests/end-to-end/api/abac.ts
📚 Learning: 2025-09-16T22:08:51.490Z
Learnt from: CR
Repo: RocketChat/Rocket.Chat PR: 0
File: .cursor/rules/playwright.mdc:0-0
Timestamp: 2025-09-16T22:08:51.490Z
Learning: Applies to apps/meteor/tests/e2e/**/*.{ts,tsx,js,jsx} : Follow DRY by extracting reusable logic into helper functions or page objects
Applied to files:
apps/meteor/tests/end-to-end/api/abac.ts
📚 Learning: 2025-11-07T14:50:33.544Z
Learnt from: KevLehman
Repo: RocketChat/Rocket.Chat PR: 37423
File: packages/i18n/src/locales/en.i18n.json:18-18
Timestamp: 2025-11-07T14:50:33.544Z
Learning: Rocket.Chat settings: in apps/meteor/ee/server/settings/abac.ts, the Abac_Cache_Decision_Time_Seconds setting uses invalidValue: 0 as the fallback when ABAC is unlicensed. With a valid license, admins can still set the value to 0 to intentionally disable the ABAC decision cache.
Applied to files:
apps/meteor/tests/end-to-end/api/abac.tsapps/meteor/ee/server/api/abac/index.ts
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: 📦 Build Packages
- GitHub Check: CodeQL-Build
- GitHub Check: CodeQL-Build
🔇 Additional comments (1)
apps/meteor/ee/server/api/abac/index.ts (1)
240-240: Inconsistency verified: AI summary does not match actual code changes.The verification confirms the concern raised. Analysis shows:
- License declarations at lines 35, 79, 113, 182, 206: All
license: ['abac']declarations remain present- ABAC_Enabled guards: Remain at lines 41-43, 92-94, 119-121, 192-194, 217-219
- Marked changes at lines 240, 260, 280: These are blank lines added for formatting within GET endpoints that do NOT have ABAC_Enabled guards
The marked GET endpoints (lines 240-250, 260-270, 280-290) appear to be missing ABAC_Enabled guards, while POST/PUT endpoints retain them. This suggests a partial change. Confirm whether this represents the intended state or if the PR description and actual implementation have diverged.
MartinSchoeler
force-pushed
the
geat/ABAC-admin-room-att
branch
from
d6dad24 to
4ac1edc
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (3)
packages/i18n/src/locales/en.i18n.json (3)
26-26: Good fix — duplicate word removed.The “rooms rooms” typo is resolved here. Thanks.
28-28: Trim the extra space before the question mark.There’s an extra space before “?”.
Apply:
- "ABAC_Delete_room_attribute_content": "Are you sure you want to delete <bold>{{attributeName}}</bold> ?<br/>Existing rooms will not be affected as it is not currently assigned to any.", + "ABAC_Delete_room_attribute_content": "Are you sure you want to delete <bold>{{attributeName}}</bold>?<br/>Existing rooms will not be affected as it is not currently assigned to any.",
37-37: Align label with key semantics (“Remove” vs “Delete”).Use “Remove” to match the key and UI copy; tests also expect a “Remove” label for this action. Based on learnings.
Apply:
- "ABAC_Remove_attribute": "Delete attribute value", + "ABAC_Remove_attribute": "Remove attribute value",
🧹 Nitpick comments (3)
apps/meteor/client/lib/queryKeys.ts (1)
121-127: Avoid leakingundefinedinto query keysEverywhere else we skip the optional payload when it’s not provided, while this helper always appends a trailing
undefined. That still works, but keeping the key shape stable helps cache lookups stay predictable and aligns with the existing pattern in this file.Apply this diff:
roomAttributes: { all: () => [...ABACQueryKeys.all, 'room-attributes'] as const, - roomAttributesList: (query?: PaginatedRequest) => [...ABACQueryKeys.roomAttributes.all(), 'room-attributes-list', query] as const, + roomAttributesList: (query?: PaginatedRequest) => + (query + ? ([...ABACQueryKeys.roomAttributes.all(), 'room-attributes-list', query] as const) + : ([...ABACQueryKeys.roomAttributes.all(), 'room-attributes-list'] as const)),packages/i18n/src/locales/en.i18n.json (2)
34-35: Optional copy polish for clarity.Tighten phrasing; no behavioral impact.
- "ABAC_Edit_attribute_description": "Attribute values cannot be edited, but can be added or deleted.", + "ABAC_Edit_attribute_description": "Attribute values cannot be edited; you can add or delete them.", - "ABAC_New_attribute_description": "Create an attribute that can later be assigned to rooms.", + "ABAC_New_attribute_description": "Create an attribute that can be assigned to rooms.",
24-24: Optional casing consistency.Consider “Room attributes” to mirror “Subject attributes” (ABAC_Attributes). If this is a page title, keeping Title Case is also fine — just ensure consistency across ABAC sections.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Jira integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
⛔ Files ignored due to path filters (1)
apps/meteor/client/views/admin/ABAC/__snapshots__/AdminABACRoomAttributesForm.spec.tsx.snapis excluded by!**/*.snap
📒 Files selected for processing (15)
apps/meteor/client/lib/queryKeys.ts(1 hunks)apps/meteor/client/views/admin/ABAC/AdminABACPage.tsx(3 hunks)apps/meteor/client/views/admin/ABAC/AdminABACRoomAttributeMenu.tsx(1 hunks)apps/meteor/client/views/admin/ABAC/AdminABACRoomAttributes.tsx(1 hunks)apps/meteor/client/views/admin/ABAC/AdminABACRoomAttributesForm.spec.tsx(4 hunks)apps/meteor/client/views/admin/ABAC/AdminABACRoomAttributesForm.tsx(2 hunks)apps/meteor/client/views/admin/ABAC/AdminABACTabs.tsx(1 hunks)apps/meteor/client/views/admin/ABAC/RoomAttributesContextualBar.tsx(1 hunks)apps/meteor/client/views/admin/ABAC/RoomAttributesContextualBarWithData.tsx(1 hunks)apps/meteor/client/views/admin/ABAC/hooks/useIsABACAvailable.tsx(1 hunks)apps/meteor/client/views/admin/ABAC/useRoomAttributeOptions.spec.tsx(1 hunks)apps/meteor/client/views/admin/ABAC/useRoomAttributeOptions.tsx(1 hunks)apps/meteor/client/views/admin/sidebarItems.ts(1 hunks)apps/meteor/ee/server/api/abac/schemas.ts(1 hunks)packages/i18n/src/locales/en.i18n.json(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (10)
- apps/meteor/client/views/admin/ABAC/hooks/useIsABACAvailable.tsx
- apps/meteor/client/views/admin/ABAC/AdminABACRoomAttributesForm.spec.tsx
- apps/meteor/client/views/admin/ABAC/AdminABACTabs.tsx
- apps/meteor/client/views/admin/ABAC/AdminABACRoomAttributes.tsx
- apps/meteor/client/views/admin/sidebarItems.ts
- apps/meteor/client/views/admin/ABAC/useRoomAttributeOptions.spec.tsx
- apps/meteor/client/views/admin/ABAC/AdminABACRoomAttributeMenu.tsx
- apps/meteor/client/views/admin/ABAC/RoomAttributesContextualBarWithData.tsx
- apps/meteor/client/views/admin/ABAC/RoomAttributesContextualBar.tsx
- apps/meteor/client/views/admin/ABAC/useRoomAttributeOptions.tsx
🧰 Additional context used
🧠 Learnings (5)
📓 Common learnings
Learnt from: KevLehman
Repo: RocketChat/Rocket.Chat PR: 37303
File: apps/meteor/tests/end-to-end/api/abac.ts:1125-1137
Timestamp: 2025-10-27T14:38:46.994Z
Learning: In Rocket.Chat ABAC feature, when ABAC is disabled globally (ABAC_Enabled setting is false), room-level ABAC attributes are not evaluated when changing room types. This means converting a private room to public will succeed even if the room has ABAC attributes, as long as the global ABAC setting is disabled.
Learnt from: KevLehman
Repo: RocketChat/Rocket.Chat PR: 37299
File: apps/meteor/ee/server/lib/ldap/Manager.ts:438-454
Timestamp: 2025-10-24T17:32:05.348Z
Learning: In Rocket.Chat, ABAC attributes can only be set on private rooms and teams (type 'p'), not on public rooms (type 'c'). Therefore, when checking for ABAC-protected rooms/teams during LDAP sync or similar operations, it's sufficient to query only private rooms using methods like `findPrivateRoomsByIdsWithAbacAttributes`.
📚 Learning: 2025-10-30T19:30:46.541Z
Learnt from: MartinSchoeler
Repo: RocketChat/Rocket.Chat PR: 37244
File: apps/meteor/client/views/admin/ABAC/AdminABACRoomAttributesForm.spec.tsx:125-146
Timestamp: 2025-10-30T19:30:46.541Z
Learning: In the AdminABACRoomAttributesForm component (apps/meteor/client/views/admin/ABAC/AdminABACRoomAttributesForm.tsx), the first attribute value field is mandatory and does not have a Remove button. Only additional values beyond the first have Remove buttons. This means trashButtons[0] corresponds to the second value's Remove button, not the first value's.
Applied to files:
apps/meteor/client/views/admin/ABAC/AdminABACRoomAttributesForm.tsxapps/meteor/client/views/admin/ABAC/AdminABACPage.tsxpackages/i18n/src/locales/en.i18n.json
📚 Learning: 2025-10-24T17:32:05.348Z
Learnt from: KevLehman
Repo: RocketChat/Rocket.Chat PR: 37299
File: apps/meteor/ee/server/lib/ldap/Manager.ts:438-454
Timestamp: 2025-10-24T17:32:05.348Z
Learning: In Rocket.Chat, ABAC attributes can only be set on private rooms and teams (type 'p'), not on public rooms (type 'c'). Therefore, when checking for ABAC-protected rooms/teams during LDAP sync or similar operations, it's sufficient to query only private rooms using methods like `findPrivateRoomsByIdsWithAbacAttributes`.
Applied to files:
apps/meteor/client/lib/queryKeys.tsapps/meteor/client/views/admin/ABAC/AdminABACPage.tsx
📚 Learning: 2025-10-27T14:38:46.994Z
Learnt from: KevLehman
Repo: RocketChat/Rocket.Chat PR: 37303
File: apps/meteor/tests/end-to-end/api/abac.ts:1125-1137
Timestamp: 2025-10-27T14:38:46.994Z
Learning: In Rocket.Chat ABAC feature, when ABAC is disabled globally (ABAC_Enabled setting is false), room-level ABAC attributes are not evaluated when changing room types. This means converting a private room to public will succeed even if the room has ABAC attributes, as long as the global ABAC setting is disabled.
Applied to files:
apps/meteor/client/lib/queryKeys.tsapps/meteor/client/views/admin/ABAC/AdminABACPage.tsxpackages/i18n/src/locales/en.i18n.json
📚 Learning: 2025-11-07T14:50:33.544Z
Learnt from: KevLehman
Repo: RocketChat/Rocket.Chat PR: 37423
File: packages/i18n/src/locales/en.i18n.json:18-18
Timestamp: 2025-11-07T14:50:33.544Z
Learning: Rocket.Chat settings: in apps/meteor/ee/server/settings/abac.ts, the Abac_Cache_Decision_Time_Seconds setting uses invalidValue: 0 as the fallback when ABAC is unlicensed. With a valid license, admins can still set the value to 0 to intentionally disable the ABAC decision cache.
Applied to files:
apps/meteor/ee/server/api/abac/schemas.ts
🧬 Code graph analysis (1)
apps/meteor/client/views/admin/ABAC/AdminABACRoomAttributesForm.tsx (2)
packages/livechat/src/components/Button/index.tsx (1)
Button(34-99)packages/livechat/src/components/ButtonGroup/index.tsx (1)
ButtonGroup(8-14)
🔇 Additional comments (2)
apps/meteor/ee/server/api/abac/schemas.ts (1)
67-69: Type definition now matches the runtime schemaMaking
key/valuesoptional in the generic keeps the TypeScript surface aligned with the AJV rules. Looks good.apps/meteor/client/views/admin/ABAC/AdminABACRoomAttributesForm.tsx (1)
70-138: Form wiring and validation look solidNice job tying the Save button to the generated
formIdand hardening the add-value guard—submission and error handling behave cleanly.
MartinSchoeler
force-pushed
the
geat/ABAC-admin-room-att
branch
from
4ac1edc to
e07c7a9
Compare
MartinSchoeler
force-pushed
the
geat/ABAC-admin-room-att
branch
from
e07c7a9 to
facb802
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/meteor/ee/server/api/abac/schemas.ts (1)
56-69: Schema inconsistency: TypeScript type declaresoffsetandcountas required, but runtime validation allows them to be absent.The TypeScript generic at line 67 indicates
offset: number; count: number(no?), suggesting these are required parameters. However, theGetAbacAttributesQueryJSON schema (lines 56-65) lacks arequiredfield, meaning AJV will accept requests whereoffsetandcountare missing. This mismatch can lead to runtime errors when code expects these values to always be present.Apply this diff to align runtime validation with the TypeScript type:
const GetAbacAttributesQuery = { type: 'object', properties: { key: { type: 'string', minLength: 1, pattern: ATTRIBUTE_KEY_PATTERN }, values: { type: 'string', minLength: 1, pattern: ATTRIBUTE_KEY_PATTERN }, offset: { type: 'number' }, count: { type: 'number' }, }, + required: ['offset', 'count'], additionalProperties: false, };
♻️ Duplicate comments (3)
apps/meteor/client/views/admin/ABAC/AdminABACPage.tsx (1)
30-42: Contextual bar never fully closes.
handleCloseContextualbarnavigates withcontext: ''/id: ''. Because the render guard only checkscontext !== undefined, the dialog remains mounted with an empty body after the user closes the bar. Please clear those params instead of setting empty strings (or tighten the guard) so the bar is removed.Apply this diff:
router.navigate( { name: 'admin-ABAC', - params: { ...router.getRouteParameters(), context: '', id: '' }, + params: { ...router.getRouteParameters(), context: undefined, id: undefined }, }, { replace: true }, );apps/meteor/client/views/admin/ABAC/RoomAttributesContextualBar.tsx (2)
49-51: Potential issue with endpoint instantiation whenattributeIdis undefined.When
attributeIdisundefined, the endpoint is instantiated with_id: ''. While the endpoint won't be called in this case (due to the check on Line 58), the empty string parameter might cause issues with URL construction or validation.Consider conditionally instantiating the endpoint or providing a fallback:
const updateAttribute = useEndpoint('PUT', '/v1/abac/attributes/:_id', { - _id: attributeId ?? '', + _id: attributeId || 'placeholder', });
80-82: Closing the form on error prevents users from correcting mistakes.The
onSettledcallback closes the contextual bar regardless of success or failure. When an error occurs, users lose their form data and cannot easily retry. Consider only closing on success.Apply this diff to close only on success:
onSuccess: () => { if (attributeId) { dispatchToastMessage({ type: 'success', message: t('ABAC_Attribute_updated', { attributeName: getValues('name') }) }); } else { dispatchToastMessage({ type: 'success', message: t('ABAC_Attribute_created', { attributeName: getValues('name') }) }); } + queryClient.invalidateQueries({ queryKey: ABACQueryKeys.roomAttributes.all() }); + onClose(); }, onError: (error) => { dispatchToastMessage({ type: 'error', message: error }); }, - onSettled: () => { - queryClient.invalidateQueries({ queryKey: ABACQueryKeys.roomAttributes.all() }); - onClose(); - },
🧹 Nitpick comments (1)
apps/meteor/client/views/admin/ABAC/RoomAttributesContextualBarWithData.tsx (1)
15-19: Consider a non-zerostaleTimefor better performance.Setting
staleTime: 0marks data as immediately stale, causing refetches on every mount, focus, or reconnect. Since attribute data is invalidated explicitly after mutations (inRoomAttributesContextualBar), a reasonable cache time (e.g., 30 seconds) would reduce unnecessary API calls without sacrificing data freshness.Apply this diff to set a 30-second stale time:
const { data, isLoading, isFetching } = useQuery({ queryKey: ABACQueryKeys.roomAttributes.attribute(id), queryFn: () => getAttributes(), - staleTime: 0, + staleTime: 30_000, });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Jira integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
⛔ Files ignored due to path filters (1)
apps/meteor/client/views/admin/ABAC/__snapshots__/AdminABACRoomAttributesForm.spec.tsx.snapis excluded by!**/*.snap
📒 Files selected for processing (14)
apps/meteor/client/lib/queryKeys.ts(1 hunks)apps/meteor/client/views/admin/ABAC/AdminABACPage.tsx(3 hunks)apps/meteor/client/views/admin/ABAC/AdminABACRoomAttributeMenu.tsx(1 hunks)apps/meteor/client/views/admin/ABAC/AdminABACRoomAttributes.tsx(1 hunks)apps/meteor/client/views/admin/ABAC/AdminABACRoomAttributesForm.spec.tsx(4 hunks)apps/meteor/client/views/admin/ABAC/AdminABACRoomAttributesForm.tsx(2 hunks)apps/meteor/client/views/admin/ABAC/AdminABACTabs.tsx(1 hunks)apps/meteor/client/views/admin/ABAC/RoomAttributesContextualBar.tsx(1 hunks)apps/meteor/client/views/admin/ABAC/RoomAttributesContextualBarWithData.tsx(1 hunks)apps/meteor/client/views/admin/ABAC/hooks/useIsABACAvailable.tsx(1 hunks)apps/meteor/client/views/admin/ABAC/useRoomAttributeOptions.spec.tsx(1 hunks)apps/meteor/client/views/admin/ABAC/useRoomAttributeOptions.tsx(1 hunks)apps/meteor/client/views/admin/sidebarItems.ts(1 hunks)apps/meteor/ee/server/api/abac/schemas.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (7)
- apps/meteor/client/views/admin/ABAC/hooks/useIsABACAvailable.tsx
- apps/meteor/client/views/admin/ABAC/useRoomAttributeOptions.tsx
- apps/meteor/client/lib/queryKeys.ts
- apps/meteor/client/views/admin/ABAC/useRoomAttributeOptions.spec.tsx
- apps/meteor/client/views/admin/sidebarItems.ts
- apps/meteor/client/views/admin/ABAC/AdminABACTabs.tsx
- apps/meteor/client/views/admin/ABAC/AdminABACRoomAttributeMenu.tsx
🧰 Additional context used
🧠 Learnings (15)
📓 Common learnings
Learnt from: KevLehman
Repo: RocketChat/Rocket.Chat PR: 37303
File: apps/meteor/tests/end-to-end/api/abac.ts:1125-1137
Timestamp: 2025-10-27T14:38:46.994Z
Learning: In Rocket.Chat ABAC feature, when ABAC is disabled globally (ABAC_Enabled setting is false), room-level ABAC attributes are not evaluated when changing room types. This means converting a private room to public will succeed even if the room has ABAC attributes, as long as the global ABAC setting is disabled.
Learnt from: KevLehman
Repo: RocketChat/Rocket.Chat PR: 37299
File: apps/meteor/ee/server/lib/ldap/Manager.ts:438-454
Timestamp: 2025-10-24T17:32:05.348Z
Learning: In Rocket.Chat, ABAC attributes can only be set on private rooms and teams (type 'p'), not on public rooms (type 'c'). Therefore, when checking for ABAC-protected rooms/teams during LDAP sync or similar operations, it's sufficient to query only private rooms using methods like `findPrivateRoomsByIdsWithAbacAttributes`.
📚 Learning: 2025-10-30T19:30:46.541Z
Learnt from: MartinSchoeler
Repo: RocketChat/Rocket.Chat PR: 37244
File: apps/meteor/client/views/admin/ABAC/AdminABACRoomAttributesForm.spec.tsx:125-146
Timestamp: 2025-10-30T19:30:46.541Z
Learning: In the AdminABACRoomAttributesForm component (apps/meteor/client/views/admin/ABAC/AdminABACRoomAttributesForm.tsx), the first attribute value field is mandatory and does not have a Remove button. Only additional values beyond the first have Remove buttons. This means trashButtons[0] corresponds to the second value's Remove button, not the first value's.
Applied to files:
apps/meteor/client/views/admin/ABAC/AdminABACRoomAttributesForm.spec.tsxapps/meteor/client/views/admin/ABAC/RoomAttributesContextualBar.tsxapps/meteor/client/views/admin/ABAC/AdminABACRoomAttributesForm.tsxapps/meteor/client/views/admin/ABAC/AdminABACPage.tsxapps/meteor/client/views/admin/ABAC/AdminABACRoomAttributes.tsxapps/meteor/client/views/admin/ABAC/RoomAttributesContextualBarWithData.tsx
📚 Learning: 2025-09-16T22:08:51.490Z
Learnt from: CR
Repo: RocketChat/Rocket.Chat PR: 0
File: .cursor/rules/playwright.mdc:0-0
Timestamp: 2025-09-16T22:08:51.490Z
Learning: Applies to apps/meteor/tests/e2e/**/*.spec.ts : Use descriptive test names that clearly communicate expected behavior
Applied to files:
apps/meteor/client/views/admin/ABAC/AdminABACRoomAttributesForm.spec.tsx
📚 Learning: 2025-09-16T22:08:51.490Z
Learnt from: CR
Repo: RocketChat/Rocket.Chat PR: 0
File: .cursor/rules/playwright.mdc:0-0
Timestamp: 2025-09-16T22:08:51.490Z
Learning: Applies to apps/meteor/tests/e2e/**/*.{ts,tsx} : Avoid using page.locator(); prefer semantic locators like page.getByRole, page.getByLabel, page.getByText, and page.getByTitle
Applied to files:
apps/meteor/client/views/admin/ABAC/AdminABACRoomAttributesForm.spec.tsx
📚 Learning: 2025-09-16T22:08:51.490Z
Learnt from: CR
Repo: RocketChat/Rocket.Chat PR: 0
File: .cursor/rules/playwright.mdc:0-0
Timestamp: 2025-09-16T22:08:51.490Z
Learning: Applies to apps/meteor/tests/e2e/**/*.spec.ts : Use expect matchers (toEqual, toContain, toBeTruthy, toHaveLength, etc.) instead of assert statements
Applied to files:
apps/meteor/client/views/admin/ABAC/AdminABACRoomAttributesForm.spec.tsx
📚 Learning: 2025-09-16T22:08:51.490Z
Learnt from: CR
Repo: RocketChat/Rocket.Chat PR: 0
File: .cursor/rules/playwright.mdc:0-0
Timestamp: 2025-09-16T22:08:51.490Z
Learning: Applies to apps/meteor/tests/e2e/**/*.{ts,tsx,js,jsx} : Write concise, technical TypeScript/JavaScript with accurate typing
Applied to files:
apps/meteor/client/views/admin/ABAC/AdminABACRoomAttributesForm.spec.tsx
📚 Learning: 2025-09-16T22:08:51.490Z
Learnt from: CR
Repo: RocketChat/Rocket.Chat PR: 0
File: .cursor/rules/playwright.mdc:0-0
Timestamp: 2025-09-16T22:08:51.490Z
Learning: Applies to apps/meteor/tests/e2e/**/*.{ts,tsx,js,jsx} : Avoid code comments in the implementation
Applied to files:
apps/meteor/client/views/admin/ABAC/AdminABACRoomAttributesForm.spec.tsx
📚 Learning: 2025-09-16T22:08:51.490Z
Learnt from: CR
Repo: RocketChat/Rocket.Chat PR: 0
File: .cursor/rules/playwright.mdc:0-0
Timestamp: 2025-09-16T22:08:51.490Z
Learning: Applies to apps/meteor/tests/e2e/**/*.spec.ts : Maintain test isolation between test cases
Applied to files:
apps/meteor/client/views/admin/ABAC/AdminABACRoomAttributesForm.spec.tsx
📚 Learning: 2025-09-16T22:08:51.490Z
Learnt from: CR
Repo: RocketChat/Rocket.Chat PR: 0
File: .cursor/rules/playwright.mdc:0-0
Timestamp: 2025-09-16T22:08:51.490Z
Learning: Applies to apps/meteor/tests/e2e/**/*.spec.ts : Ensure tests run reliably in parallel without shared state conflicts
Applied to files:
apps/meteor/client/views/admin/ABAC/AdminABACRoomAttributesForm.spec.tsx
📚 Learning: 2025-09-16T22:08:51.490Z
Learnt from: CR
Repo: RocketChat/Rocket.Chat PR: 0
File: .cursor/rules/playwright.mdc:0-0
Timestamp: 2025-09-16T22:08:51.490Z
Learning: Applies to apps/meteor/tests/e2e/**/*.spec.ts : Group related tests in the same file
Applied to files:
apps/meteor/client/views/admin/ABAC/AdminABACRoomAttributesForm.spec.tsx
📚 Learning: 2025-09-16T22:08:51.490Z
Learnt from: CR
Repo: RocketChat/Rocket.Chat PR: 0
File: .cursor/rules/playwright.mdc:0-0
Timestamp: 2025-09-16T22:08:51.490Z
Learning: Applies to apps/meteor/tests/e2e/**/*.{ts,tsx} : Follow the Page Object Model pattern consistently
Applied to files:
apps/meteor/client/views/admin/ABAC/AdminABACRoomAttributesForm.spec.tsx
📚 Learning: 2025-09-23T19:22:59.217Z
Learnt from: dougfabris
Repo: RocketChat/Rocket.Chat PR: 36987
File: apps/meteor/tests/e2e/page-objects/fragments/room-toolbar.ts:10-20
Timestamp: 2025-09-23T19:22:59.217Z
Learning: In Playwright e2e tests, prefer stable selectors like data-qa-id attributes over localized text in getByRole() or getByText() calls to prevent test failures when UI language changes. Test translations separately by validating actual text content after ensuring UI interactions work with stable selectors.
Applied to files:
apps/meteor/client/views/admin/ABAC/AdminABACRoomAttributesForm.spec.tsx
📚 Learning: 2025-09-16T22:08:51.490Z
Learnt from: CR
Repo: RocketChat/Rocket.Chat PR: 0
File: .cursor/rules/playwright.mdc:0-0
Timestamp: 2025-09-16T22:08:51.490Z
Learning: Applies to apps/meteor/tests/e2e/**/*.spec.ts : Prefer web-first assertions (e.g., toBeVisible, toHaveText)
Applied to files:
apps/meteor/client/views/admin/ABAC/AdminABACRoomAttributesForm.spec.tsx
📚 Learning: 2025-11-07T14:50:33.544Z
Learnt from: KevLehman
Repo: RocketChat/Rocket.Chat PR: 37423
File: packages/i18n/src/locales/en.i18n.json:18-18
Timestamp: 2025-11-07T14:50:33.544Z
Learning: Rocket.Chat settings: in apps/meteor/ee/server/settings/abac.ts, the Abac_Cache_Decision_Time_Seconds setting uses invalidValue: 0 as the fallback when ABAC is unlicensed. With a valid license, admins can still set the value to 0 to intentionally disable the ABAC decision cache.
Applied to files:
apps/meteor/ee/server/api/abac/schemas.ts
📚 Learning: 2025-10-27T14:38:46.994Z
Learnt from: KevLehman
Repo: RocketChat/Rocket.Chat PR: 37303
File: apps/meteor/tests/end-to-end/api/abac.ts:1125-1137
Timestamp: 2025-10-27T14:38:46.994Z
Learning: In Rocket.Chat ABAC feature, when ABAC is disabled globally (ABAC_Enabled setting is false), room-level ABAC attributes are not evaluated when changing room types. This means converting a private room to public will succeed even if the room has ABAC attributes, as long as the global ABAC setting is disabled.
Applied to files:
apps/meteor/client/views/admin/ABAC/AdminABACPage.tsxapps/meteor/client/views/admin/ABAC/AdminABACRoomAttributes.tsx
🧬 Code graph analysis (5)
apps/meteor/client/views/admin/ABAC/RoomAttributesContextualBar.tsx (2)
apps/meteor/client/views/admin/ABAC/AdminABACRoomAttributesForm.tsx (1)
AdminABACRoomAttributesFormFormData(19-23)apps/meteor/client/lib/queryKeys.ts (1)
ABACQueryKeys(121-128)
apps/meteor/client/views/admin/ABAC/AdminABACRoomAttributesForm.tsx (2)
packages/livechat/src/components/Button/index.tsx (1)
Button(34-99)packages/livechat/src/components/ButtonGroup/index.tsx (1)
ButtonGroup(8-14)
apps/meteor/client/views/admin/ABAC/AdminABACPage.tsx (2)
packages/ui-contexts/src/index.ts (2)
useRouter(60-60)useRouteParameter(62-62)apps/meteor/client/router/page.ts (1)
Page(92-241)
apps/meteor/client/views/admin/ABAC/AdminABACRoomAttributes.tsx (2)
apps/meteor/client/components/GenericTable/hooks/usePagination.ts (1)
usePagination(11-40)apps/meteor/client/lib/queryKeys.ts (1)
ABACQueryKeys(121-128)
apps/meteor/client/views/admin/ABAC/RoomAttributesContextualBarWithData.tsx (1)
apps/meteor/client/lib/queryKeys.ts (1)
ABACQueryKeys(121-128)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: 📦 Build Packages
- GitHub Check: CodeQL-Build
- GitHub Check: CodeQL-Build
🔇 Additional comments (10)
apps/meteor/client/views/admin/ABAC/AdminABACRoomAttributesForm.spec.tsx (1)
138-138: LGTM! Selector updated to use i18n key.The test now uses the
ABAC_Remove_attributei18n key instead of the literal "Remove" text, making it more resilient to localization changes.Also applies to: 161-161, 214-214, 256-256
apps/meteor/client/views/admin/ABAC/RoomAttributesContextualBarWithData.tsx (1)
21-23: LGTM! Loading state handling is appropriate.Checking both
isLoadingandisFetchingensures the skeleton displays during initial load and subsequent refetches, providing consistent user feedback.apps/meteor/client/views/admin/ABAC/AdminABACPage.tsx (1)
65-68: LGTM! Tab-based content rendering.The page correctly renders
AdminABACSettingsfor the settings tab andAdminABACRoomAttributesfor the room-attributes tab.apps/meteor/client/views/admin/ABAC/RoomAttributesContextualBar.tsx (1)
27-43: LGTM! Form initialization handles create and edit modes correctly.The form defaults are properly set based on whether
attributeDatais present, with locked attributes populated from existing values in edit mode.apps/meteor/client/views/admin/ABAC/AdminABACRoomAttributes.tsx (3)
25-26: LGTM! Debounced search with appropriate delay.The 200ms debounce prevents excessive API calls during typing while maintaining responsive search UX.
Also applies to: 42-49
66-68: LGTM! ABAC availability check gates attribute creation.The "New Attribute" button is correctly disabled when ABAC is unavailable, preventing invalid operations.
84-92: LGTM! Clean table rendering with proper data display.Attributes are rendered with their keys and comma-separated values, with action menus for each row.
apps/meteor/client/views/admin/ABAC/AdminABACRoomAttributesForm.tsx (3)
26-26: LGTM! Improved type safety foronSaveprop.The prop type now explicitly expects
AdminABACRoomAttributesFormFormData, providing better type checking and IDE support.
51-51: LGTM! Improved form submission pattern.Using
formIdwith theformattribute on the submit button allows proper form submission even when the button is outside the form element, following best HTML practices.Also applies to: 73-73, 133-133
92-127: LGTM! Consolidated attribute value rendering with proper accessibility.The unified rendering of locked and regular attributes is clean, with appropriate remove button logic (first field mandatory if no locked attributes) and a reasonable 10-value limit. The explicit
titleattribute improves accessibility.
apps/meteor/client/views/admin/ABAC/hooks/useIsABACAvailable.tsx
Outdated
Show resolved
Hide resolved
apps/meteor/client/views/admin/ABAC/hooks/useIsABACAvailable.tsx
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
apps/meteor/client/views/admin/ABAC/AdminABACPage.tsx (1)
30-42: Contextual bar closing issue already flagged.This issue with setting
context: ''andid: ''instead of clearing them has already been flagged in a previous review. The render guard on line 70 checkscontext !== undefined, which means empty strings will keep the bar mounted.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Jira integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (5)
apps/meteor/client/views/admin/ABAC/AdminABACPage.tsx(3 hunks)apps/meteor/client/views/admin/ABAC/AdminABACRoomAttributes.tsx(1 hunks)apps/meteor/client/views/admin/ABAC/hooks/useIsABACAvailable.tsx(1 hunks)apps/meteor/client/views/admin/ABAC/useRoomAttributeOptions.spec.tsx(1 hunks)apps/meteor/client/views/admin/ABAC/useRoomAttributeOptions.tsx(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- apps/meteor/client/views/admin/ABAC/useRoomAttributeOptions.spec.tsx
- apps/meteor/client/views/admin/ABAC/hooks/useIsABACAvailable.tsx
🧰 Additional context used
🧠 Learnings (5)
📓 Common learnings
Learnt from: KevLehman
Repo: RocketChat/Rocket.Chat PR: 37303
File: apps/meteor/tests/end-to-end/api/abac.ts:1125-1137
Timestamp: 2025-10-27T14:38:46.994Z
Learning: In Rocket.Chat ABAC feature, when ABAC is disabled globally (ABAC_Enabled setting is false), room-level ABAC attributes are not evaluated when changing room types. This means converting a private room to public will succeed even if the room has ABAC attributes, as long as the global ABAC setting is disabled.
Learnt from: KevLehman
Repo: RocketChat/Rocket.Chat PR: 37299
File: apps/meteor/ee/server/lib/ldap/Manager.ts:438-454
Timestamp: 2025-10-24T17:32:05.348Z
Learning: In Rocket.Chat, ABAC attributes can only be set on private rooms and teams (type 'p'), not on public rooms (type 'c'). Therefore, when checking for ABAC-protected rooms/teams during LDAP sync or similar operations, it's sufficient to query only private rooms using methods like `findPrivateRoomsByIdsWithAbacAttributes`.
📚 Learning: 2025-10-30T19:30:46.541Z
Learnt from: MartinSchoeler
Repo: RocketChat/Rocket.Chat PR: 37244
File: apps/meteor/client/views/admin/ABAC/AdminABACRoomAttributesForm.spec.tsx:125-146
Timestamp: 2025-10-30T19:30:46.541Z
Learning: In the AdminABACRoomAttributesForm component (apps/meteor/client/views/admin/ABAC/AdminABACRoomAttributesForm.tsx), the first attribute value field is mandatory and does not have a Remove button. Only additional values beyond the first have Remove buttons. This means trashButtons[0] corresponds to the second value's Remove button, not the first value's.
Applied to files:
apps/meteor/client/views/admin/ABAC/AdminABACRoomAttributes.tsxapps/meteor/client/views/admin/ABAC/AdminABACPage.tsxapps/meteor/client/views/admin/ABAC/useRoomAttributeOptions.tsx
📚 Learning: 2025-10-27T14:38:46.994Z
Learnt from: KevLehman
Repo: RocketChat/Rocket.Chat PR: 37303
File: apps/meteor/tests/end-to-end/api/abac.ts:1125-1137
Timestamp: 2025-10-27T14:38:46.994Z
Learning: In Rocket.Chat ABAC feature, when ABAC is disabled globally (ABAC_Enabled setting is false), room-level ABAC attributes are not evaluated when changing room types. This means converting a private room to public will succeed even if the room has ABAC attributes, as long as the global ABAC setting is disabled.
Applied to files:
apps/meteor/client/views/admin/ABAC/AdminABACRoomAttributes.tsxapps/meteor/client/views/admin/ABAC/AdminABACPage.tsx
📚 Learning: 2025-11-10T19:06:20.146Z
Learnt from: MartinSchoeler
Repo: RocketChat/Rocket.Chat PR: 37408
File: apps/meteor/client/views/admin/ABAC/useRoomAttributeOptions.tsx:53-69
Timestamp: 2025-11-10T19:06:20.146Z
Learning: In the Rocket.Chat repository, do not provide suggestions or recommendations about code sections marked with TODO comments. The maintainers have already identified these as future work and external reviewers lack the full context about implementation plans and timing.
Applied to files:
apps/meteor/client/views/admin/ABAC/useRoomAttributeOptions.tsx
📚 Learning: 2025-10-24T17:32:05.348Z
Learnt from: KevLehman
Repo: RocketChat/Rocket.Chat PR: 37299
File: apps/meteor/ee/server/lib/ldap/Manager.ts:438-454
Timestamp: 2025-10-24T17:32:05.348Z
Learning: In Rocket.Chat, ABAC attributes can only be set on private rooms and teams (type 'p'), not on public rooms (type 'c'). Therefore, when checking for ABAC-protected rooms/teams during LDAP sync or similar operations, it's sufficient to query only private rooms using methods like `findPrivateRoomsByIdsWithAbacAttributes`.
Applied to files:
apps/meteor/client/views/admin/ABAC/useRoomAttributeOptions.tsx
🧬 Code graph analysis (3)
apps/meteor/client/views/admin/ABAC/AdminABACRoomAttributes.tsx (2)
apps/meteor/client/components/GenericTable/hooks/usePagination.ts (1)
usePagination(11-40)apps/meteor/client/lib/queryKeys.ts (1)
ABACQueryKeys(121-128)
apps/meteor/client/views/admin/ABAC/AdminABACPage.tsx (2)
packages/ui-contexts/src/index.ts (2)
useRouter(60-60)useRouteParameter(62-62)apps/meteor/client/router/page.ts (1)
Page(92-241)
apps/meteor/client/views/admin/ABAC/useRoomAttributeOptions.tsx (2)
packages/ui-contexts/src/index.ts (3)
useRouter(60-60)useSetModal(68-68)useToastMessageDispatch(75-75)apps/meteor/client/lib/queryKeys.ts (1)
ABACQueryKeys(121-128)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: 📦 Build Packages
- GitHub Check: CodeQL-Build
- GitHub Check: CodeQL-Build
🔇 Additional comments (5)
apps/meteor/client/views/admin/ABAC/useRoomAttributeOptions.tsx (5)
1-10: LGTM!All imports are appropriate for the functionality implemented in this hook.
12-21: LGTM!Hook setup is clean with appropriate endpoint configurations and proper use of path parameters.
22-34: LGTM!The edit action correctly uses
useEffectEventand navigates to the edit context with appropriate parameters.
36-48: LGTM!The delete mutation is properly configured with success/error handling, appropriate query invalidation, and modal cleanup in
onSettled.
91-100: LGTM!The menu items are properly structured with appropriate disabled states, danger styling for the delete action, and correct onClick handlers.
apps/meteor/client/views/admin/ABAC/AdminABACRoomAttributes.tsx
Outdated
Show resolved
Hide resolved
apps/meteor/client/views/admin/ABAC/RoomAttributesContextualBar.tsx
Outdated
Show resolved
Hide resolved
Proposed changes (including videos or screenshots)
Issue(s)
ABAC-49
Steps to test or reproduce
Further comments
Summary by CodeRabbit
New Features
Bug Fixes
Tests